.github/workflows/test.yml (1)

16-17: Extract bot actor into environment variable for maintainability

As noted in a previous review, hardcoding the bot name across multiple jobs can lead to maintenance issues if the bot’s username changes. Define a workflow- or job-level env var (e.g., TRUSTED_BOT: codegen-sh[bot]) and update the if to:

env:
  TRUSTED_BOT: codegen-sh[bot]
if: github.actor != env.TRUSTED_BOT

codegen-on-oss/codegen_on_oss/analysis/__init__.py (1)

1-1: Remove redundant import comment.

The comment is redundant as it simply states what the code does, which is already clear from the import statement itself.

-# Add import for the harness integration
 from codegen_on_oss.analysis.harness_integration import CodebaseAnalysisHarness

codegen-on-oss/codegen_on_oss/context_server/__init__.py (1)

1-1: Enhance module docstring with more details.

The current module docstring is minimal and doesn't fully explain the purpose or functionality of the module.

-"""Context server module for CodeContextRetrievalServer."""
+"""Provides a server implementation for retrieving and managing code context.
+
+Exports the FastAPI application instance and server startup functionality."""

codegen-on-oss/codegen_on_oss/cli.py (5)

158-158: Enhance server startup logging

The startup log message should include all operational parameters for better diagnostics.

- logger.info(f"Starting CodeContextRetrievalServer on {host}:{port}")
+ logger.info(f"Starting CodeContextRetrievalServer on {host}:{port} (debug={debug})")

---

135-136: ⚠️ Potential issue

Security: Binding to all interfaces by default

Binding to "0.0.0.0" by default exposes the server to all network interfaces, including potentially untrusted networks, increasing the attack surface.

@click.option(
    "--host",
    type=str,
-    default="0.0.0.0",
+    default="127.0.0.1",
    help="Host to bind to",
)

And in the function definition:

def serve(
-    host: str = "0.0.0.0",
+    host: str = "127.0.0.1",
    port: int = 8000,
    debug: bool = False,
):

Also applies to: 150-150

🧰 Tools

🪛 Ruff (0.8.2)

135-135: Possible binding to all interfaces

(S104)

---

154-156: 🛠️ Refactor suggestion

Improve the command docstring

The docstring for the serve command lacks details about the server's purpose and capabilities.

"""
- Start the CodeContextRetrievalServer.
+ Start the CodeContextRetrievalServer for providing code context lookups over HTTP.
+ 
+ The server provides RESTful API endpoints for repository analysis, context management,
+ and agent execution, forming a backend for code context retrieval operations.
"""

---

157-157: 🛠️ Refactor suggestion

Inconsistent logging configuration

Unlike other commands in this file, the serve command doesn't configure error file logging.

+ error_output_path = cachedir / "server_errors.log"
+ logger.add(error_output_path, format="{time: HH:mm:ss} {level} {message}", level="ERROR")
- logger.add(sys.stdout, level="DEBUG" if debug else "INFO")
+ logger.add(sys.stdout, format="{time: HH:mm:ss} {level} {message}", level="DEBUG" if debug else "INFO")

---

159-159: ⚠️ Potential issue

Add error handling for server startup

The server startup doesn't include error handling for potential failures like port conflicts.

- start_server(host=host, port=port)
+ try:
+     start_server(host=host, port=port)
+ except Exception as e:
+     logger.error(f"Failed to start server: {str(e)}")
+     sys.exit(1)

codegen-on-oss/examples/codebase_analysis_example.py (4)

36-36: Use a more descriptive variable name for analysis results

The generic variable name 'results' doesn't indicate what type of analysis results are being stored.

- results = harness.analyze_codebase()
+ codebase_analysis = harness.analyze_codebase()

Update the references to this variable in the rest of the code as well.

---

48-50: Use named expression to simplify code

You can use assignment expressions (the walrus operator) to simplify this code.

- diff = harness.get_diff()
- if diff:
+ if diff := harness.get_diff():

---

15-17: 🛠️ Refactor suggestion

Enhance main function docstring with workflow details

The docstring should explain what the function does, its workflow, and expected outputs.

"""
- Main function to demonstrate the CodebaseAnalysisHarness.
+ Analyze a GitHub repository and generate analysis reports.
+ 
+ Requires a repository name in the format 'owner/repo'. Optionally accepts a specific commit hash.
+ Generates a JSON analysis report and saves it to a file in the current directory.
+ Creates a snapshot of the repository state and retrieves any diff from the base commit.
"""

---

23-23: ⚠️ Potential issue

Add input validation for repository name

Direct use of command line input without validation could allow path traversal when creating the output file.

repo_full_name = sys.argv[1]
+ # Validate the repository name format
+ if not all(part.isalnum() or part.startswith(".") for part in repo_full_name.split("/")) or repo_full_name.count("/") != 1:
+     print("Invalid repository name. Expected format: owner/repo")
+     sys.exit(1)

codegen-on-oss/examples/context_server_example.py (1)

42-54: 🛠️ Refactor suggestion

Add timeout and implement retry logic for analyze request

For robust API interaction, add timeout and retry logic to handle temporary failures.

# Analyze repository
print("\nAnalyzing repository...")
- response = requests.post(
-     f"{server_url}/analyze",
-     json={
-         "repo_full_name": repo_full_name,
-         "commit": commit,
-     },
- )
+ 
+ max_retries = 3
+ retry_delay = 2
+ 
+ for attempt in range(max_retries):
+     try:
+         response = requests.post(
+             f"{server_url}/analyze",
+             json={
+                 "repo_full_name": repo_full_name,
+                 "commit": commit,
+             },
+             timeout=30,  # 30 second timeout for potentially long analysis
+         )
+         break
+     except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e:
+         if attempt < max_retries - 1:
+             print(f"Request failed, retrying in {retry_delay} seconds... ({attempt+1}/{max_retries})")
+             time.sleep(retry_delay)
+             retry_delay *= 2  # Exponential backoff
+         else:
+             print(f"Error: Failed to connect to server after {max_retries} attempts.")
+             sys.exit(1)

With this change, you'll need to re-add the import time that was previously removed.

🧰 Tools

🪛 Ruff (0.8.2)

44-44: Probable use of requests call without timeout

(S113)

---

51-51: Blank line contains whitespace

Remove whitespace from blank line

(W293)

codegen-on-oss/README.md (1)

13-41: 🛠️ Refactor suggestion

Add security & production-deployment sections to README
The new server-side features introduce network-facing attack-surface, but the README still lacks guidance on authentication, rate-limiting, TLS, logging, and monitoring. Re-introducing the “Security Considerations” and “Production Deployment” subsections (as already suggested in a prior review) would help downstream users run the service safely in production.

codegen-on-oss/codegen_on_oss/snapshot/context_snapshot.py (3)

134-136: Consider custom exception for missing snapshot
Raising raw ValueError loses semantic meaning. A dedicated SnapshotNotFoundError improves caller-side handling.

🧰 Tools

🪛 Ruff (0.8.2)

135-135: Avoid specifying long messages outside the exception class

(TRY003)

---

136-136: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

82-85: ⚠️ Potential issue

Replace os.system("zip …") with zipfile.ZipFile
Shelling-out for zipping is (1) platform-specific, (2) unchecked for non-zero exit codes, and (3) vulnerable to argument-injection if temp_dir ever contains spaces or shell metacharacters. A pure-Python solution is safer and already recommended by multiple earlier reviews.

-import os
-…
-os.system(f"cd {temp_dir} && zip -r {snapshot_path} .")
+import zipfile
+…
+with zipfile.ZipFile(snapshot_path, "w", zipfile.ZIP_DEFLATED) as zf:
+    for p in temp_path.rglob("*"):
+        if p.is_file():
+            zf.write(p, p.relative_to(temp_path))

🧰 Tools

🪛 Ruff (0.8.2)

84-84: Starting a process with a shell, possible injection detected

(S605)

---

85-85: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

140-142: ⚠️ Potential issue

Avoid os.system("unzip …") for extraction
Same security, portability and error-handling issues as with the zip call. Use zipfile.ZipFile.extractall instead.

-import os
-…
-os.system(f"cd {temp_dir} && unzip {zip_path}")
+import zipfile
+…
+with zipfile.ZipFile(zip_path, "r") as zf:
+    zf.extractall(temp_path)

🧰 Tools

🪛 Ruff (0.8.2)

141-141: Starting a process with a shell, possible injection detected

(S605)

---

142-142: Blank line contains whitespace

Remove whitespace from blank line

(W293)

codegen-on-oss/codegen_on_oss/analysis/harness_integration.py (2)

170-175: files_in_patch incorrectly preserves a/ & b/ prefixes
Splitting on the first '/' leaves leading a/ or b/ in the filename, and the naive check misses renamed files. Consider regex or difflib.

---

155-157: ⚠️ Potential issue

Use shlex.split (or list args) for git diff invocation
Splitting the command with .split() breaks on paths with spaces and is vulnerable to injection when repo_path is user-supplied.

-import subprocess
-…
-diff_output = subprocess.check_output(diff_cmd.split()).decode()
+import shlex
+…
+diff_output = subprocess.check_output(shlex.split(diff_cmd)).decode()

🧰 Tools

🪛 Ruff (0.8.2)

156-156: subprocess call: check for execution of untrusted input

(S603)

codegen-on-oss/codegen_on_oss/context_server/server.py (1)

137-156: Listing snapshots via a dummy harness is inefficient – refactor to a static helper

Multiple prior reviews already flagged this; the current implementation still spins up a fake repository analysis just to enumerate S3 keys. That is wasteful and may fail if network access is blocked.

Either:

• add CodebaseContextSnapshot.list_snapshots_for_repo(repo_name: Optional[str]) (static/class method), or
• call a thin helper in BucketStore directly.

Marking as duplicate of earlier feedback.

🧰 Tools

🪛 Ruff (0.8.2)

138-138: Use X | Y for type annotations

Convert to X | Y

(UP007)

---

141-141: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

144-144: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

156-156: Consider moving this statement to an else block

(TRY300)

codegen-on-oss/codegen_on_oss/snapshot/__init__.py (1)

1-4: Consider adding a package docstring.

The import and export of CodebaseContextSnapshot look good, following Python package conventions by using __all__ to explicitly define the public API. However, adding a package-level docstring would improve discoverability and understanding of this module's purpose.

+"""Package for managing codebase context snapshots with S3 integration."""
 # Add import for the context snapshot
 from codegen_on_oss.snapshot.context_snapshot import CodebaseContextSnapshot
 
 __all__ = ["CodebaseContextSnapshot"]

codegen-on-oss/codegen_on_oss/analysis/__init__.py (1)

2-4: Consider adding a package docstring.

The import and export of CodebaseAnalysisHarness correctly follow Python package conventions with __all__, but adding a package-level docstring would improve documentation and discoverability.

+"""Package for analyzing codebases with comprehensive metrics and reporting."""
 from codegen_on_oss.analysis.harness_integration import CodebaseAnalysisHarness

 __all__ = ["CodebaseAnalysisHarness"]

codegen-on-oss/examples/context_server_example.py (3)

6-10: Remove unused import and organize imports

The time module is imported but not used in the script.

import json
import requests
import sys
- import time

🧰 Tools

🪛 Ruff (0.8.2)

9-9: time imported but unused

Remove unused import: time

(F401)

---

51-73: Clean up whitespace in blank lines

There are multiple instances of blank lines with whitespace in this file.

You can clean up these blank lines throughout the file by removing any whitespace characters.

🧰 Tools

🪛 Ruff (0.8.2)

51-51: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

55-55: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

62-62: Probable use of requests call without timeout

(S113)

---

69-69: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

73-73: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

104-111: Enhance error handling for agent run response

Add more detailed error handling and response validation for the agent run response.

if response.status_code != 200:
    print(f"Error running agent: {response.text}")
else:
    agent_result = response.json()
    print("Agent run complete!")
-   if "edited_files" in agent_result:
-       print(f"Edited files: {agent_result['edited_files']}")
+   # Validate and display response fields
+   if not isinstance(agent_result, dict):
+       print("Warning: Agent returned unexpected response format")
+   else:
+       if "edited_files" in agent_result:
+           print(f"Edited files: {', '.join(agent_result['edited_files'])}")
+       if "diff" in agent_result:
+           print("\nDiff generated by agent:")
+           print(agent_result["diff"])
+       if "result" in agent_result:
+           print("\nAgent result:")
+           print(agent_result["result"])

codegen-on-oss/codegen_on_oss/snapshot/context_snapshot.py (1)

25-47: Minor typing & path-safety nits
• typing.Dict/List are deprecated; prefer built-ins (dict, list).
• repo_full_name can include slashes – ensure S3 key sanitisation or use them deliberately as prefixes.

🧰 Tools

🪛 Ruff (0.8.2)

28-28: Use X | Y for type annotations

Convert to X | Y

(UP007)

codegen-on-oss/codegen_on_oss/analysis/harness_integration.py (1)

6-12: Remove unused imports & deprecated typing aliases
pprint, random, Set, Tuple, Union are unused; typing.Dict/List are deprecated. Cleaning these up eliminates dead code and suppresses Ruff warnings.

🧰 Tools

🪛 Ruff (0.8.2)

7-7: pprint imported but unused

Remove unused import: pprint

(F401)

---

8-8: random imported but unused

Remove unused import: random

(F401)

---

12-12: typing.Dict is deprecated, use dict instead

(UP035)

---

12-12: typing.List is deprecated, use list instead

(UP035)

---

12-12: typing.Set is deprecated, use set instead

(UP035)

---

12-12: typing.Tuple is deprecated, use tuple instead

(UP035)

---

12-12: typing.Set imported but unused

Remove unused import

(F401)

---

12-12: typing.Tuple imported but unused

Remove unused import

(F401)

---

12-12: typing.Union imported but unused

Remove unused import

(F401)

codegen-on-oss/codegen_on_oss/context_server/server.py (3)

6-14: Prune unused imports & adopt modern typing syntax

json, os, List, Union, and Query are never referenced, and the Dict/List generics coming from typing have been superseded by the built-in dict, list and the | union operator (PEP 604). Cleaning these up will remove dead code and let Ruff pass without ignores.

-import json
-import os
-from typing import Any, Dict, List, Optional, Union
-from fastapi import FastAPI, HTTPException, Query
+from typing import Any, Optional, Dict
+from fastapi import FastAPI, HTTPException

(If you still need any of those names in the future, re-import them where they’re actually required.)

🧰 Tools

🪛 Ruff (0.8.2)

6-6: json imported but unused

Remove unused import: json

(F401)

---

7-7: os imported but unused

Remove unused import: os

(F401)

---

8-8: typing.Dict is deprecated, use dict instead

(UP035)

---

8-8: typing.List is deprecated, use list instead

(UP035)

---

8-8: typing.List imported but unused

Remove unused import

(F401)

---

8-8: typing.Union imported but unused

Remove unused import

(F401)

---

10-10: fastapi.Query imported but unused

Remove unused import: fastapi.Query

(F401)

---

48-55: Reconsider permissive CORS settings

allow_origins=["*"] combined with open credentials/methods exposes every endpoint—including /agent/run and /snapshot/*—to any web page.
Unless the server is strictly internal, tighten CORS (e.g. read the allowed origins from an env-var) or add authentication middleware before going to production.

---

162-187: Factor out repeated “create harness → do work” logic

/analyze, /snapshot/create, /agent/run all duplicate:

harness = CodebaseAnalysisHarness.from_repo(...)

A small dependency-injection helper (or FastAPI Depends) would:

• centralise parameter validation,
• allow caching of harnesses per (repo, commit),
• make unit-testing endpoints trivial.

Not urgent, but will pay off as endpoints grow.

🧰 Tools

🪛 Ruff (0.8.2)

166-166: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

169-169: Blank line contains whitespace

Remove whitespace from blank line

(W293)

---

183-183: Consider moving this statement to an else block

(TRY300)

---

186-186: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro